-
Notifications
You must be signed in to change notification settings - Fork 297
enhance debug log #151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
enhance debug log #151
Conversation
|
@henryxwong is attempting to deploy a commit to the Goshen Labs Team on Vercel. A member of the Team first needs to authorize it. |
Greptile OverviewGreptile SummaryThis PR enhances CLI debug logging in
This fits into the engine layer where all AI backends shell out to provider CLIs; adding debug logs here makes verbose mode more informative across the whole CLI. Key issues to address before merge:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Base as base.ts
participant Log as logger.ts
participant Proc as ChildProcess/Bun.spawn
Caller->>Base: execCommand(cmd,args,workDir,env,stdin?)
Base->>Log: logDebug("$ <formatted command>")
Base->>Proc: spawn/spawnArgs
Base->>Proc: collect stdout/stderr
Proc-->>Base: stdout, stderr, exitCode
Base->>Log: logDebug("[stdout] ...")
Base->>Log: logDebug("[stderr] ...")
Base-->>Caller: {stdout, stderr, exitCode}
Caller->>Base: execCommandStreaming(cmd,args,...,onLine)
Base->>Log: logDebug("$ <formatted command>")
Base->>Proc: spawn
Proc-->>Base: data events
Base->>Caller: onLine(line)
Proc-->>Base: close(exitCode)
Base->>Log: logDebug("[stdout]/[stderr] (remaining buffers)")
Base-->>Caller: {exitCode}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 2 comments
| // Log the command being executed | ||
| logDebug(`$ ${formatCommand(command, args)}`); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sensitive data leakage
execCommand/execCommandStreaming now unconditionally logDebug the full command line (and later stdout/stderr). If args/stdout include secrets (API keys, tokens, auth headers, model provider keys, etc.), those will be printed in verbose mode and can end up in CI logs/bug reports. Consider redacting common secret patterns (or only logging the binary name + arg count) before emitting debug logs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/engines/base.ts
Line: 56:58
Comment:
**Sensitive data leakage**
`execCommand`/`execCommandStreaming` now unconditionally `logDebug` the full command line (and later stdout/stderr). If args/stdout include secrets (API keys, tokens, auth headers, model provider keys, etc.), those will be printed in verbose mode and can end up in CI logs/bug reports. Consider redacting common secret patterns (or only logging the binary name + arg count) before emitting debug logs.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
In Prompt To Fix With AIThis is a comment left during a code review.
Path: cli/src/engines/base.ts
Line: 320:345
Comment:
**Inconsistent streaming logging**
In `execCommandStreaming`, the Bun path returns `{ exitCode }` without ever calling `logCommandOutput`, so verbose users get command/output logs on Node but not on Bun. If the intent is “show the command line being executed and their output” across runtimes, this needs a Bun-side equivalent (e.g., buffer the output while still streaming lines) or document that Bun streaming won’t log output.
How can I resolve this? If you propose a fix, please make it concise. |
Enhance debug log, to show the command line being executed and their output.